feat: custom d3 number locale #20075

ebaratte · 2022-05-16T09:17:44Z

SUMMARY

This PR is an attempt to provide a way of customising the format of the numbers and currency displayed in superset frontend.
It is linked to (at least) issues #3972, #18938, #11328, #8913
Please also look at PR #17796, which provides a different approach to the same issue.

Design choice:

the user can change the following d3 number formatting parameters: decimal, thousands, grouping, currency, numerals, percent, minus and nan
this change does not include the time formatting parameters, since I believe it to be a different issue
unlike feat: Create D3NumberFormatters with active Locale #17796, it does not tie the format to the LANGUAGES config key: the user can use different parameters for the frontend language and number formatting

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After:

TESTING INSTRUCTIONS

in config.py, add a D3_FORMAT dictionary with d3 locale settings
check the number formatting on any chart

ADDITIONAL INFORMATION

Has associated issue: [feature] Global customise-able d3 locale #3972, Can superset fromat currency dynamically according to the user's setting #18938, Enable to change currency format from dollars to euros #11328, Issue with Currency format D3 #8913
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-05-16T09:46:00Z

Codecov Report

Merging #20075 (44ff829) into master (314987f) will increase coverage by 0.00%.
The diff coverage is 87.50%.

❗ Current head 44ff829 differs from pull request most recent head c28c849. Consider uploading reports for the commit c28c849 to get more accurate results

@@           Coverage Diff           @@
##           master   #20075   +/-   ##
=======================================
  Coverage   68.02%   68.03%           
=======================================
  Files        1936     1937    +1     
  Lines       74929    74943   +14     
  Branches     8141     8141           
=======================================
+ Hits        50974    50987   +13     
  Misses      21870    21870           
- Partials     2085     2086    +1

Flag	Coverage Δ
hive	`53.01% <100.00%> (+<0.01%)`	⬆️
javascript	`54.30% <80.00%> (+<0.01%)`	⬆️
mysql	`78.80% <100.00%> (+<0.01%)`	⬆️
postgres	`78.88% <100.00%> (+<0.01%)`	⬆️
presto	`52.93% <100.00%> (+<0.01%)`	⬆️
python	`82.67% <100.00%> (+<0.01%)`	⬆️
sqlite	`77.39% <100.00%> (+<0.01%)`	⬆️
unit	`52.82% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...board/components/nativeFilters/FilterBar/index.tsx	`50.53% <ø> (ø)`
superset-frontend/src/preamble.ts	`0.00% <0.00%> (ø)`
superset-frontend/src/setup/setupFormatters.ts	`0.00% <ø> (ø)`
superset-frontend/src/types/bootstrapTypes.ts	`100.00% <ø> (ø)`
superset/views/base.py	`76.87% <ø> (ø)`
...d/components/DashboardBuilder/DashboardBuilder.tsx	`73.26% <50.00%> (-0.74%)`	⬇️
...perset-ui-chart-controls/src/utils/D3Formatting.ts	`100.00% <100.00%> (ø)`
...perset-ui-core/src/number-format/D3FormatConfig.ts	`100.00% <100.00%> (ø)`
...-core/src/number-format/NumberFormatterRegistry.ts	`100.00% <100.00%> (ø)`
.../number-format/NumberFormatterRegistrySingleton.ts	`100.00% <100.00%> (ø)`
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

k7daniel · 2022-05-18T12:59:26Z

This is very cool. Would be great to just set D3_FORMAT and pass that in to the docker image.

SergeRomankevich · 2022-05-19T04:46:39Z

Tables with different currencies are crying aside and waiting for the merge

enryls · 2022-06-25T17:38:51Z

Great feature! Hope to see it merged soon

pm-at · 2022-07-05T11:40:14Z

Is there a way to bring this issue to the notice of reviewers?

KarenWeis · 2022-08-09T16:20:55Z

Are there plans to merge or include in 3.0 release?

theYaro · 2022-10-17T13:24:15Z

Waiting for this merge.

pm-at · 2023-01-12T07:10:35Z

I'm sorry for repeated nudge, but this is a really helpful feature. Waiting for this PR to be merged..

MarkusBellmann · 2023-04-12T15:01:19Z

Please, we really need this feature!

lf-floriandin · 2023-04-13T06:37:53Z

Would also love if this get reviewed and merged!

villebro · 2023-04-13T07:48:51Z

@ebaratte would you be able to rebase this PR to fix the conflicts?

ebaratte · 2023-04-13T14:51:03Z

I rebased the PR, we should be good to go.

Pedrolrbr · 2023-04-20T14:15:28Z

@justinpark @eschutho @kgabryje @villebro any updates?

villebro

I know that Superset is badly lacking in this space, but the main issue I have with this approach is the fact that it assumes the deployment as a whole speaks one single locale. As we start to prepare for making Superset more localizable (see e.g. a WIP PR of mine that adds timezone support to the chart data API: #23511), it would be nice if we could start making gradual progress towards having user or chart specific locales. Let's say we want to have one chart that shows euros, and another that shows US dollars. This would not be possible with this setting.

Having said that, I think this approach is good to cover use cases where the entire deployment wants to "speak" only one specific locale, as long as we leave open the door for making this overridable per user/chart. And based on my review this doesn't paint us into any corner, so I think this looks good. Thoughts @ebaratte @Pedrolrbr @kgabryje @rusackas ?

villebro · 2023-04-21T06:38:13Z

superset/config.py

+# "currency": ["$", ""] # - currency prefix/suffix strings (e.g., ["$", ""])
+# }
+# https://github.com/d3/d3-format/blob/main/README.md#formatLocale
+D3_FORMAT: Dict[str, Any] = {}


nit: as TypedDict is now a default thing in our currently supported Python versions, I'd prefer to create a D3Format(TypedDict) for this.

Sure, I updated the type definition

villebro · 2023-04-21T06:39:43Z

superset-frontend/src/types/bootstrapTypes.ts

@@ -150,6 +151,7 @@ export interface CommonBootstrapData {
 extra_sequential_color_schemes: SequentialSchemeConfig[];
 theme_overrides: JsonObject;
 menu_data: MenuData;
+ d3_format: Partial<FormatLocaleDefinition>;


Do we really need Partial here? It appears that the added dict in config.py defines all properties of FormatLocaleDefinition. If not, let's make sure the type in config.py reflects the partialness (this could be done by adding total=False to the Python class that's extending TypedDict).

I think we do; the way the code is set up, a user can specify a subset of the formatting options in config.py (total=False in the type definition).
The default values are set in the front (D3FormatConfig.ts, and setD3Format() in NumberFormatterRegistry.ts:52)

ebaratte · 2023-04-25T10:22:30Z

I know that Superset is badly lacking in this space, but the main issue I have with this approach is the fact that it assumes the deployment as a whole speaks one single locale. As we start to prepare for making Superset more localizable (see e.g. a WIP PR of mine that adds timezone support to the chart data API: #23511), it would be nice if we could start making gradual progress towards having user or chart specific locales. Let's say we want to have one chart that shows euros, and another that shows US dollars. This would not be possible with this setting.

Having said that, I think this approach is good to cover use cases where the entire deployment wants to "speak" only one specific locale, as long as we leave open the door for making this overridable per user/chart. And based on my review this doesn't paint us into any corner, so I think this looks good. Thoughts @ebaratte @Pedrolrbr @kgabryje @rusackas ?

This is indeed the general idea of this PR: set default values for the whole instance. This is especially relevant for the currency, since there is no proper way at the moment to change the default currency, and displaying an incorrect currency makes the charts incorrect - whereas number formatting is more of a cosmetic issue. Obviously, it assumes that all the data is in a single currency.

It shouldn't prevent local overrides, like:

setting a specific currency on a chart (feat(formatters): [WIP] Adding a few currency formatters #22374)
using a number format that depends on the current locale (feat: Create D3NumberFormatters with active Locale #17796), or via user settings

villebro · 2023-04-25T14:26:01Z

This is indeed the general idea of this PR: set default values for the whole instance. This is especially relevant for the currency, since there is no proper way at the moment to change the default currency, and displaying an incorrect currency makes the charts incorrect - whereas number formatting is more of a cosmetic issue. Obviously, it assumes that all the data is in a single currency.

It shouldn't prevent local overrides, like:

setting a specific currency on a chart (feat(formatters): [WIP] Adding a few currency formatters #22374)

using a number format that depends on the current locale (feat: Create D3NumberFormatters with active Locale #17796), or via user settings

Ok you don't need to convince me more @ebaratte 🙂 Looks good to go to me, but there appears to be some frontend UTs that are failing. Please take a look so we can get this in 👍

villebro

LGTM once the tests are passing.

ebaratte · 2023-04-26T08:15:43Z

Rebased to fix the tests (#23805)

k7daniel · 2023-04-26T10:13:21Z

Yay! Can't wait to test this out!!

rusackas · 2023-04-27T17:45:43Z

Hoping to test this on an ephemeral build, but then I can't wait to click the Merge button on this!!!! 🚀

rusackas · 2023-04-27T21:52:47Z

/testenv up

rusackas · 2023-04-27T21:53:33Z

Sorry for all the noise on the thread, folks! I'm trying to troubleshoot the ephemeral environment builds, so it needs a little kicking ;)

github-actions · 2023-04-27T21:54:36Z

@rusackas Ephemeral environment spinning up at http://54.200.4.245:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas · 2023-05-02T18:29:29Z

Had a nice long chat with @mistercrunch about this approach and a couple other approaches to improving currency handling in Superset. At the end of the day, I think all three have their place. Expect some more proposals and/or PRs in the future. In the meantime, this looks ready to go. Thanks for the PR!!!! Hitting the merge button!

github-actions · 2023-05-02T18:30:10Z

Ephemeral environment shutdown and build artifacts deleted.

cwegener · 2023-05-03T02:01:21Z

Just cross-linking the alternative designs for future reference (somehow Github doesn't place links from Discussion threads for issue mentions into the issue log automatically ...)
#23684

phifa · 2023-06-12T11:42:15Z

So I am not familiar with D3_FORMAT. Can someone tell me what I need to add to config.py? I want this number to be displayed in a european format, like this: 579 781 or like this 579.781,000. Thanks a ton!

cwegener · 2023-06-12T22:10:13Z

@phifa
According to the documentation, this config should be what is needed:

D3_FORMAT = {
    "decimal": ".",
    "thousands": ",",
    "grouping": [3],
    "currency": ["€", ""]
}

dab21rus · 2023-06-19T08:18:59Z

@phifa According to the documentation, this config should be what is needed:
D3_FORMAT = {
    "decimal": ".",
    "thousands": ",",
    "grouping": [3],
    "currency": ["€", ""]
}

Does it work on version 2.1.0 ? Nothing changes for me.

cwegener · 2023-06-19T08:32:48Z

Does it work on version 2.1.0 ? Nothing changes for me.

No. This PR is not part of 2.1

This PR is only available in the master branch.

pull-request-size bot added size/M size/L and removed size/M labels May 16, 2022

rusackas requested review from kgabryje and villebro August 9, 2022 16:24

ebaratte force-pushed the master branch from fb1a536 to 06810a5 Compare April 13, 2023 14:50

ebaratte force-pushed the master branch 2 times, most recently from d12cff2 to ec9d938 Compare April 13, 2023 15:50

rusackas mentioned this pull request Apr 13, 2023

feat(formatters): [WIP] Adding a few currency formatters #22374

Closed

9 tasks

ebaratte force-pushed the master branch 2 times, most recently from 6b1b2c5 to bbfff77 Compare April 14, 2023 15:42

rusackas requested review from eschutho and justinpark April 18, 2023 15:29

villebro reviewed Apr 21, 2023

View reviewed changes

ebaratte force-pushed the master branch 2 times, most recently from 0446aea to 606bbf1 Compare April 25, 2023 10:00

villebro approved these changes Apr 25, 2023

View reviewed changes

feat: custom d3 number locale

c28c849

ebaratte force-pushed the master branch from 606bbf1 to c28c849 Compare April 26, 2023 08:09

apache deleted a comment from github-actions bot Apr 27, 2023

rusackas merged commit a170ae4 into apache:master May 2, 2023

rusackas mentioned this pull request May 4, 2023

feat: Create D3NumberFormatters with active Locale #17796

Closed

9 tasks

mostafa-k-m pushed a commit to sylndr/superset that referenced this pull request Dec 6, 2023

feat: custom d3 number locale (apache#20075)

e1f5599

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom d3 number locale #20075

feat: custom d3 number locale #20075

ebaratte commented May 16, 2022

codecov bot commented May 16, 2022 •

edited

Loading

k7daniel commented May 18, 2022

SergeRomankevich commented May 19, 2022

enryls commented Jun 25, 2022

pm-at commented Jul 5, 2022 •

edited

Loading

KarenWeis commented Aug 9, 2022

theYaro commented Oct 17, 2022

pm-at commented Jan 12, 2023

MarkusBellmann commented Apr 12, 2023

lf-floriandin commented Apr 13, 2023

villebro commented Apr 13, 2023

ebaratte commented Apr 13, 2023

Pedrolrbr commented Apr 20, 2023

villebro left a comment •

edited

Loading

villebro Apr 21, 2023

ebaratte Apr 25, 2023

villebro Apr 21, 2023 •

edited

Loading

ebaratte Apr 25, 2023

ebaratte commented Apr 25, 2023

villebro commented Apr 25, 2023

villebro left a comment

ebaratte commented Apr 26, 2023

k7daniel commented Apr 26, 2023

rusackas commented Apr 27, 2023

rusackas commented Apr 27, 2023

rusackas commented Apr 27, 2023

github-actions bot commented Apr 27, 2023

rusackas commented May 2, 2023

github-actions bot commented May 2, 2023

cwegener commented May 3, 2023

phifa commented Jun 12, 2023

cwegener commented Jun 12, 2023

dab21rus commented Jun 19, 2023

cwegener commented Jun 19, 2023

feat: custom d3 number locale #20075

feat: custom d3 number locale #20075

Conversation

ebaratte commented May 16, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 16, 2022 • edited Loading

Codecov Report

k7daniel commented May 18, 2022

SergeRomankevich commented May 19, 2022

enryls commented Jun 25, 2022

pm-at commented Jul 5, 2022 • edited Loading

KarenWeis commented Aug 9, 2022

theYaro commented Oct 17, 2022

pm-at commented Jan 12, 2023

MarkusBellmann commented Apr 12, 2023

lf-floriandin commented Apr 13, 2023

villebro commented Apr 13, 2023

ebaratte commented Apr 13, 2023

Pedrolrbr commented Apr 20, 2023

villebro left a comment • edited Loading

Choose a reason for hiding this comment

villebro Apr 21, 2023

Choose a reason for hiding this comment

ebaratte Apr 25, 2023

Choose a reason for hiding this comment

villebro Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

ebaratte Apr 25, 2023

Choose a reason for hiding this comment

ebaratte commented Apr 25, 2023

villebro commented Apr 25, 2023

villebro left a comment

Choose a reason for hiding this comment

ebaratte commented Apr 26, 2023

k7daniel commented Apr 26, 2023

rusackas commented Apr 27, 2023

rusackas commented Apr 27, 2023

rusackas commented Apr 27, 2023

github-actions bot commented Apr 27, 2023

rusackas commented May 2, 2023

github-actions bot commented May 2, 2023

cwegener commented May 3, 2023

phifa commented Jun 12, 2023

cwegener commented Jun 12, 2023

dab21rus commented Jun 19, 2023

cwegener commented Jun 19, 2023

codecov bot commented May 16, 2022 •

edited

Loading

pm-at commented Jul 5, 2022 •

edited

Loading

villebro left a comment •

edited

Loading

villebro Apr 21, 2023 •

edited

Loading